Implement `black_box` using intrinsic #87916

nbdd0121 · 2021-08-10T14:33:20Z

Introduce black_box intrinsic, as suggested in #87590 (comment).

This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity.

cc @Amanieu as this is related to inline assembly
cc @bjorn3 for rustc_codegen_cranelift changes
cc @RalfJung as this affects MIRI

r? @nagisa I suppose

rust-highfive · 2021-08-10T14:33:22Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

compiler/rustc_codegen_llvm/src/intrinsic.rs

nagisa · 2021-08-10T14:53:52Z

Looks pretty nice. And in the future we have freedom to implement this in a better or otherwise backend specific manner better.

nagisa · 2021-08-10T14:54:28Z

@bors delegate=nbdd0121

(feel free to r=nagisa once the unwrap is bug!ified)

bors · 2021-08-10T14:54:31Z

✌️ @nbdd0121 can now approve this pull request

nbdd0121 · 2021-08-10T15:06:54Z

@bors r=nagisa

bors · 2021-08-10T15:07:02Z

📌 Commit 3cf2a69 has been approved by nagisa

library/core/src/hint.rs

RalfJung · 2021-08-10T16:58:15Z

library/core/src/hint.rs

-    dummy
+    #[cfg(not(bootstrap))]
+    {
+        crate::intrinsics::black_box(dummy)


Longer-term we might even want to directly reexport the intrinsic instead of using a wrapper function... but that's blocked on the fn ptr reification PR.

I guess that's also blocked by path component stability

Well, we already reexport transmute and probably will reexport copy/copy_nonoverlapping soon (we did but it was reverted due to the fn ptr reification problem).

So, while the lack of path component stability is a problem, it's not necessarily a blocker.

Anyway, even the exported function is currently still unstable; this is a discussion to be had at stabilization time.

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

nbdd0121 · 2021-08-10T18:11:53Z

@bors r=nagisa

bors · 2021-08-10T18:11:54Z

📌 Commit f98d540 has been approved by nagisa

Implement `black_box` using intrinsic Introduce `black_box` intrinsic, as suggested in rust-lang#87590 (comment). This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity. cc `@Amanieu` as this is related to inline assembly cc `@bjorn3` for rustc_codegen_cranelift changes cc `@RalfJung` as this affects MIRI r? `@nagisa` I suppose

JohnTitor · 2021-08-11T19:35:28Z

Possibly failed in rollup: #87939 (comment)
@bors r-

nbdd0121 · 2021-08-11T20:39:31Z

failures:

---- [ui] ui/sanitize/memory.rs stdout ----

error: error pattern ' in the stack frame of function 'random'' not found!
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a"
stdout:
------------------------------------------


------------------------------------------
stderr:
------------------------------------------
==10866==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5565d1c6fb44  (/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a+0x67b44)
    #1 0x7f93163a10b2  (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #2 0x5565d1c111cd  (/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a+0x91cd)

  Uninitialized value was created by an allocation of 'r.i' in the stack frame of function 'main'
    #0 0x5565d1c6fa40  (/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a+0x67a40)

SUMMARY: MemorySanitizer: use-of-uninitialized-value (/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a+0x67b44) 

------------------------------------------

It seems to me that the codegen is better with the intrinsic approach and a memcpy is eliminated from random, so the place where uninit values end up getting used becomes main.

Previously it's like (expressed in MIR):

fn black_box(_1: T) -> T {
    let mut _0: T; // return place
    let mut _2: &mut T;

	bb0: {
	      _2 = &mut _1;
		  llvm_asm!(""::"r"(_2):"memory":"volatile");
	      _0 = move _1; // LLVM can't optimize out
		  return;
	}
}

The intrinsic approach it's codegenned more like:

fn black_box(_1: T) -> T {
    let mut _0: T; // return place
    let mut _2: &mut T;

	bb0: {
		  _0 = move _1; // LLVM can optimize out
	      _2 = &mut _0;
		  llvm_asm!(""::"r"(_2):"memory":"volatile");
		  return;
	}
}

I think it'd be sufficient just to change the pattern so that "main" is also matched?

nbdd0121 · 2021-08-12T00:51:21Z

@nagisa A sanitize test needs to be fixed (last commit). PTAL

nagisa · 2021-08-12T12:12:04Z

Please squash the commits. r=me once its done.

The new implementation allows some `memcpy`s to be optimized away, so the uninit value in ui/sanitize/memory.rs is constructed directly onto the return place. Therefore the sanitizer now says that the value is allocated by `main` rather than `random`.

nbdd0121 · 2021-08-12T15:19:49Z

@bors r=nagisa

bors · 2021-08-12T15:19:50Z

📌 Commit 1fb1643 has been approved by nagisa

Implement `black_box` using intrinsic Introduce `black_box` intrinsic, as suggested in rust-lang#87590 (comment). This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity. cc `@Amanieu` as this is related to inline assembly cc `@bjorn3` for rustc_codegen_cranelift changes cc `@RalfJung` as this affects MIRI r? `@nagisa` I suppose

bors · 2021-08-12T21:04:10Z

⌛ Testing commit 1fb1643 with merge 0fa3190...

bors · 2021-08-12T23:48:11Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 0fa3190 to master...

…arth Rollup of 4 pull requests Successful merges: - rust-lang#87916 (Implement `black_box` using intrinsic) - rust-lang#87922 (Add c_enum_min_bits target spec field, use for arm-none and thumb-none targets) - rust-lang#87953 (Improve formatting of closure capture migration suggestion for multi-line closures.) - rust-lang#87965 (Silence non_fmt_panic from external macros.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned nagisa Aug 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2021

nagisa reviewed Aug 10, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/intrinsic.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2021

RalfJung reviewed Aug 10, 2021

View reviewed changes

library/core/src/hint.rs Show resolved Hide resolved

RalfJung reviewed Aug 10, 2021

View reviewed changes

bjorn3 reviewed Aug 10, 2021

View reviewed changes

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs Show resolved Hide resolved

JohnTitor mentioned this pull request Aug 11, 2021

Rollup of 8 pull requests #87939

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 11, 2021

Amanieu mentioned this pull request Aug 11, 2021

Deprecate llvm_asm! #87590

Merged

nbdd0121 force-pushed the black_box branch from 31aa207 to 6566b96 Compare August 12, 2021 15:16

nbdd0121 force-pushed the black_box branch from 6566b96 to 1fb1643 Compare August 12, 2021 15:18

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 12, 2021

Manishearth mentioned this pull request Aug 12, 2021

Rollup of 4 pull requests #87980

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 12, 2021

bors merged commit 0fa3190 into rust-lang:master Aug 12, 2021

rustbot added this to the 1.56.0 milestone Aug 12, 2021

nbdd0121 deleted the black_box branch August 12, 2021 23:49

nbdd0121 mentioned this pull request Sep 3, 2021

Tracking issue for std::hint::black_box #64102

Closed

5 tasks

Implement black_box using intrinsic #87916

Implement black_box using intrinsic #87916

Uh oh!

Conversation

nbdd0121 commented Aug 10, 2021

Uh oh!

rust-highfive commented Aug 10, 2021

Uh oh!

Uh oh!

nagisa commented Aug 10, 2021

Uh oh!

nagisa commented Aug 10, 2021

Uh oh!

bors commented Aug 10, 2021

Uh oh!

nbdd0121 commented Aug 10, 2021

Uh oh!

bors commented Aug 10, 2021

Uh oh!

Uh oh!

RalfJung Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

nbdd0121 Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

RalfJung Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nbdd0121 commented Aug 10, 2021

Uh oh!

bors commented Aug 10, 2021

Uh oh!

JohnTitor commented Aug 11, 2021

Uh oh!

nbdd0121 commented Aug 11, 2021

Uh oh!

nbdd0121 commented Aug 12, 2021

Uh oh!

nagisa commented Aug 12, 2021

Uh oh!

nbdd0121 commented Aug 12, 2021

Uh oh!

bors commented Aug 12, 2021

Uh oh!

bors commented Aug 12, 2021

Uh oh!

bors commented Aug 12, 2021

Uh oh!

Uh oh!

Implement `black_box` using intrinsic #87916

Implement `black_box` using intrinsic #87916

RalfJung Aug 10, 2021 •

edited

Loading